Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

es6 RecordArrayManager #4708

Merged
merged 1 commit into from
Jan 13, 2017
Merged

es6 RecordArrayManager #4708

merged 1 commit into from
Jan 13, 2017

Conversation

runspired
Copy link
Contributor

@runspired runspired commented Dec 9, 2016

There wasn't really a good reason for this singleton being an Ember.Object (it's not resolved / not in the container / no ember-y things in use), and converting it shaves 33% off it's instantiation time in addition to giving us nice es6 syntax (1.53ms => .97ms).

I additionally moved the instantiation of the OrderedSet used to track the RecordArrays for an internalModel into internal models directly instead of having the manager reach in. I'm slightly conflicted on this because of the resulting double-underscore for the cached property, but I prefer to leave objects in control of their own properties.

Before
es6-manager-before

After
es6-manager-after

@stefanpenner
Copy link
Member

stefanpenner commented Dec 9, 2016

This may be ok here. Although it is unclear if others don't reopen/extend this class. If that isn't the case, and the output isn't unreasonably large. This may be fine.


@runspired / @krisselden and myself should powow soon to fixup the cost centers insides of creation (of first object when mixins need application) in ember's core_object. I believe there is a relatively clear path to improving that, that would just have mega leverage across the whole ecosystem. I also believe this can be used to help bridge the ES6 <=> CoreObject worlds. As they really are not very dissimilar.

@runspired
Copy link
Contributor Author

@stefanpenner this isn't something others can extend, but it is something that could theoretically have been reached into, found, and reopened. Is there a list of ember-data addons that we generally search for private usage in? cc @bmac

@@ -24,6 +24,7 @@ export default Route.extend({
let modelName = params.modelName;
delete params.modelName;

console.time('ember-data');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ should we leave this?

@@ -33,6 +34,7 @@ export default Route.extend({
// and clutter our benchmarks and make it harder to time.
records.toArray();
heimdall.stop(token);
console.timeEnd('ember-data');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ should we leave this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found it invaluable and it's in our dummy app so it should be ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

however I also just want to bake this into heimdall.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

get _recordArrays() {
return this.__recordArrays;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

who sets __recordArrays ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps no one at the moment, but our tests often introspect state so moving it to a similar getter will let us test correctly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, sounds good.

@krisselden
Copy link
Contributor

(1.53ms => .97ms).

was this one sample?

@krisselden
Copy link
Contributor

@stefanpenner didn't you reopen this at Yapp for the subscribe stuff?

@krisselden
Copy link
Contributor

/cc @lukemelia

@stefanpenner
Copy link
Member

@stefanpenner didn't you reopen this at Yapp for the subscribe stuff?

Unclear, I do also share your concern this is risky relative to the value. (making mixin stuff faster, would have nice leverage...)

@krisselden
Copy link
Contributor

Also, the proto() is only doing work here because of lookupFactory extend.

@runspired
Copy link
Contributor Author

As far as wins go, this one is pretty small, but one of hundreds of paper cuts we die from when out together.

I don't actually think the win here is in this small perf gain as much as it is in building a core set of primitives that don't depend on Ember unless they truly need to. If Ember.Object and lookupFactory do have their costs lowered, I'm still unconvinced that this would belong remaining one.

@bmac
Copy link
Member

bmac commented Dec 12, 2016

I just checked out the 47 Ember Data Adapter addons from ember observer into a directory and grepped for RecordArrayManager and found no matches. I think its unlikely that anyone is reopening the RecordArrayManager.

I've updated my search to include the https://emberobserver.com/categories/ember-data-extensions and still found nothing that references RecordArrayManager.

@chadhietala
Copy link
Contributor

@runspired This great! Is there any way to get some more exhaustive benchmarks?

@runspired
Copy link
Contributor Author

@chadhietala the benchmark runs for repeated iterations are roughly similar. Since we strip the classCallCheck I'd be surprised if the compiled output were larger vs equal or smaller than the compiled output for the Ember.Object version.

@fivetanley
Copy link
Member

Would heavily prefer this to be feature flagged.

@bmac
Copy link
Member

bmac commented Dec 13, 2016

@fivetanley What is your thinking on why the feature flag for this change. In theory it should be invisible to users and doesn't provide and new functionality?

@runspired
Copy link
Contributor Author

runspired commented Dec 14, 2016

@bmac @fivetanley I think this is the sort of thing where if we send it out, and it breaks people's apps, its small and we just revert it. Don't think it warrants a flag.

@runspired
Copy link
Contributor Author

cc @bmac @fivetanley for review and suggested next steps.

@bmac
Copy link
Member

bmac commented Jan 13, 2017

@runspired this just needs a rebase and should be good to merge.

@runspired
Copy link
Contributor Author

@bmac rebased :)

@bmac bmac merged commit ae4aa84 into emberjs:master Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants